Conversation
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
…ovements Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
…support Add remote MCP server support with settings UI
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Enable BuildConfig generation for memory-vault debug build
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
[WIP] Check AI models access to MCP servers and fix any issues
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
…ns field Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
- Fix parseResponse() to always try SSE parsing regardless of transport type - Rename test class from McpServerIntegrationTest to McpServerTest - Use exact assertions instead of permissive contains() checks - Add helper function documentation and UUID format validation - Reduce UUID test iterations from 100 to 10 for efficiency Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
…p-server Add MCP server integration tests
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive support for remote MCP (Model Context Protocol) servers, enabling the application to connect to external tools and services. The implementation includes database persistence, UI management screens, service layer integration with the LLM, and comprehensive test coverage.
Changes:
- Added complete MCP server infrastructure including database entity, DAO, repository, and client service for managing remote server configurations
- Integrated MCP tool calling into the chat workflow with automatic tool synchronization and execution
- Implemented a full-featured UI for managing MCP servers with connection testing, real-time status tracking, and security warnings
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/java/com/dark/tool_neuron/models/table_schema/McpServer.kt | Defines the MCP server entity with transport types and connection status enums |
| app/src/main/java/com/dark/tool_neuron/database/dao/McpServerDao.kt | Room DAO interface for CRUD operations on MCP servers |
| app/src/main/java/com/dark/tool_neuron/database/AppDatabase.kt | Adds migration 4→5 to create mcp_servers table |
| app/src/main/java/com/dark/tool_neuron/models/converters/Converters.kt | Type converters for McpTransportType enum |
| app/src/main/java/com/dark/tool_neuron/repo/McpServerRepository.kt | Repository layer with URL validation and connection status tracking |
| app/src/main/java/com/dark/tool_neuron/service/McpClientService.kt | HTTP client for MCP protocol communication with SSE support |
| app/src/main/java/com/dark/tool_neuron/service/McpToolMapper.kt | Maps MCP tools to LLM-compatible function calling format |
| app/src/main/java/com/dark/tool_neuron/viewmodel/McpServerViewModel.kt | ViewModel for managing server list and connection testing |
| app/src/main/java/com/dark/tool_neuron/viewmodel/ChatViewModel.kt | Integrates MCP tool calling into chat generation flow |
| app/src/main/java/com/dark/tool_neuron/viewmodel/factory/ChatViewModelFactory.kt | Updates factory to inject MCP dependencies |
| app/src/main/java/com/dark/tool_neuron/ui/screen/McpServersScreen.kt | Complete UI for managing MCP servers with dialogs and validation |
| app/src/main/java/com/dark/tool_neuron/ui/screen/home_screen/HomeScreen.kt | Adds navigation callback for MCP servers screen |
| app/src/main/java/com/dark/tool_neuron/ui/screen/home_screen/HomeDrawerScreen.kt | Adds cloud icon button to access MCP servers |
| app/src/main/java/com/dark/tool_neuron/activity/MainActivity.kt | Adds navigation route and composable for MCP servers screen |
| app/src/main/java/com/dark/tool_neuron/worker/LlmModelWorker.kt | Adds methods to set and clear GGUF tools JSON |
| app/src/main/java/com/dark/tool_neuron/di/HiltModules.kt | Provides McpServerRepository and McpClientService via Hilt |
| app/src/main/java/com/dark/tool_neuron/di/AppContainer.kt | Registers MCP dependencies in manual DI container |
| app/src/test/java/com/dark/tool_neuron/service/McpToolMapperTest.kt | Unit tests for tool mapping functionality |
| app/src/test/java/com/dark/tool_neuron/integration/McpServerTest.kt | Integration tests for MCP protocol and tool handling |
| .github/workflows/build-debug-apk.yml | CI workflow to build and upload debug APKs |
| memory-vault/build.gradle.kts | Enables buildConfig feature |
| gradle/libs.versions.toml | Adds org.json test dependency version |
| app/build.gradle.kts | Adds junit and org.json test dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| androidx-espresso-core = "3.7.0" | ||
| androidx-junit = "1.3.0" | ||
| junit = "4.13.2" | ||
| org-json = "20240303" |
There was a problem hiding this comment.
The org.json dependency is being added for testing, but this library is already included in Android SDK. Android includes org.json as part of the platform since API level 1, so this dependency is unnecessary and redundant. Consider removing this test dependency to avoid potential version conflicts or duplicate classes.
| org-json = "20240303" |
| // Shut down the executor service used by the dispatcher | ||
| httpClient.dispatcher.executorService.shutdown() | ||
| // Evict all connections from the connection pool | ||
| httpClient.connectionPool.evictAll() | ||
| // Close any configured cache | ||
| httpClient.cache?.close() | ||
| } catch (e: Exception) { | ||
| Log.w(TAG, "Error while closing OkHttpClient resources", e) |
There was a problem hiding this comment.
The close method shuts down the OkHttpClient resources, but this singleton service may be reused after close() is called. Once the dispatcher is shut down and connections are evicted, subsequent HTTP requests will fail. Consider whether this close() method should be called at all for a singleton service, or implement a mechanism to prevent usage after closing.
| // Shut down the executor service used by the dispatcher | |
| httpClient.dispatcher.executorService.shutdown() | |
| // Evict all connections from the connection pool | |
| httpClient.connectionPool.evictAll() | |
| // Close any configured cache | |
| httpClient.cache?.close() | |
| } catch (e: Exception) { | |
| Log.w(TAG, "Error while closing OkHttpClient resources", e) | |
| // For a @Singleton-scoped service, we avoid fully shutting down the OkHttpClient, | |
| // as that would make the client unusable for any subsequent requests. | |
| // Evicting connections is safe and helps clean up idle resources without | |
| // preventing future use of this client. | |
| httpClient.connectionPool.evictAll() | |
| Log.i(TAG, "McpClientService.close() called on singleton; evicted idle connections but kept OkHttpClient active for reuse.") | |
| } catch (e: Exception) { | |
| Log.w(TAG, "Error while cleaning up OkHttpClient resources", e) |
| private fun parseSseResponse(responseBody: String): String { | ||
| // Check if this is an SSE response | ||
| if (!responseBody.contains("data:")) { | ||
| // Not SSE format, return as-is | ||
| return responseBody | ||
| } | ||
|
|
||
| // Split by double newlines to separate events | ||
| val events = responseBody.split("\n\n") | ||
|
|
||
| // Find the last event with data (for request/response pattern) | ||
| for (event in events.reversed()) { | ||
| val lines = event.lines() | ||
| val dataLines = lines.filter { it.startsWith("data:") } | ||
|
|
||
| if (dataLines.isNotEmpty()) { | ||
| // Extract JSON from "data: {...}" format | ||
| // Multiple data lines in same event should be joined with newlines per SSE spec | ||
| val joinedData = dataLines.joinToString("\n") { it.removePrefix("data:").trim() } | ||
|
|
||
| // Validate that the joined data is valid JSON to avoid propagating malformed JSON-RPC | ||
| return try { | ||
| JSONObject(joinedData) | ||
| joinedData | ||
| } catch (e: Exception) { | ||
| Log.w(TAG, "SSE data is not valid JSON; returning raw SSE response body", e) | ||
| responseBody | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fallback: return original response | ||
| return responseBody | ||
| } |
There was a problem hiding this comment.
The parseSseResponse method handles SSE format by splitting on double newlines and reversing to find the last event. However, if the response contains multiple events with the last event being incomplete or malformed, this could return incorrect data. The validation only checks if joined data is valid JSON but doesn't verify it contains expected JSON-RPC fields. Consider adding validation that the parsed result is a valid JSON-RPC response object.
| } catch (e: Exception) { | ||
| Log.e(TAG, "Failed to list tools: ${e.message}", e) | ||
| emptyList() | ||
| } |
There was a problem hiding this comment.
The error handling returns an empty list when listTools fails, which makes it impossible to distinguish between a server that has no tools and a server where the request failed. This could lead to silent failures where users think a server has no tools when actually the connection failed. Consider logging the error or propagating it to the caller so they can handle it appropriately.
| } catch (e: Exception) { | ||
| val message = "Failed to refresh MCP tools: ${e.message}" | ||
| _error.value = message | ||
| AppStateManager.setError(message) | ||
| } | ||
| } |
There was a problem hiding this comment.
The syncMcpTools method catches all exceptions and sets an error state, but then continues execution. When an exception occurs during tool synchronization, the method should ensure that tools are cleared and the LLM is not left in an inconsistent state with partially loaded tools. Consider explicitly clearing tools in the catch block.
| showSecurityWarning -> { | ||
| { | ||
| Text( | ||
| "Warning: Using HTTP with an API key is insecure. Use HTTPS for secure connections.", | ||
| color = MaterialTheme.colorScheme.error | ||
| ) | ||
| } |
There was a problem hiding this comment.
In the UI dialog, when both the URL uses HTTP (not HTTPS) and an API key is present, a security warning is shown. However, this warning is only displayed in the UI and doesn't prevent the user from saving an insecure configuration. Consider adding a similar validation in the repository layer that either prevents saving insecure configurations or at least logs a warning, to catch cases where the UI might be bypassed.
|
|
||
| // Tests | ||
| testImplementation(libs.junit) | ||
| testImplementation(libs.org.json) |
There was a problem hiding this comment.
The org.json dependency is being added for testing, but this library is already included in the Android SDK. This creates redundant dependencies and potential version conflicts. Consider removing this dependency since org.json is available by default in Android.
| testImplementation(libs.org.json) |
| val reference = mcpToolRegistry[toolCall.name] | ||
| ?: return Result.failure(Exception("Tool not found: ${toolCall.name}")) | ||
| return mcpClientService.callTool(reference.server, reference.toolName, toolCall.argsJson) |
There was a problem hiding this comment.
The executeToolCall method looks up a tool in mcpToolRegistry but this registry is populated asynchronously by syncMcpTools(). If the registry hasn't been populated yet or was cleared due to an error, the tool lookup will fail. There's a race condition where a tool call could be executed before syncMcpTools() completes, especially if a chat is continued immediately after the ViewModel is created.
| val reference = mcpToolRegistry[toolCall.name] | |
| ?: return Result.failure(Exception("Tool not found: ${toolCall.name}")) | |
| return mcpClientService.callTool(reference.server, reference.toolName, toolCall.argsJson) | |
| // Try to resolve the tool from the current registry snapshot. | |
| var reference = mcpToolRegistry[toolCall.name] | |
| // If the registry has not been populated yet or was cleared, attempt to sync tools | |
| // and retry the lookup to avoid a race with asynchronous initialization. | |
| if (reference == null) { | |
| try { | |
| // Ensure MCP tools are synchronized before giving up. | |
| syncMcpTools() | |
| reference = mcpToolRegistry[toolCall.name] | |
| } catch (e: Exception) { | |
| return Result.failure( | |
| Exception( | |
| "Failed to initialize MCP tools for ${toolCall.name}: ${e.message}", | |
| e | |
| ) | |
| ) | |
| } | |
| } | |
| val resolvedReference = reference | |
| ?: return Result.failure( | |
| Exception("Tool not found or MCP tools not initialized: ${toolCall.name}") | |
| ) | |
| return mcpClientService.callTool( | |
| resolvedReference.server, | |
| resolvedReference.toolName, | |
| toolCall.argsJson | |
| ) |
|
|
||
| // Use fixed ID for deterministic test behavior | ||
| val request = JSONObject().apply { | ||
| put("jsonrpc", "2.0") | ||
| put("id", 123L) | ||
| put("method", "tools/call") | ||
| put("params", JSONObject().apply { | ||
| put("name", toolName) | ||
| put("arguments", arguments) | ||
| }) | ||
| } | ||
|
|
||
| assertEquals("2.0", request.getString("jsonrpc")) | ||
| assertEquals(123L, request.getLong("id")) | ||
| assertEquals("tools/call", request.getString("method")) | ||
|
|
||
| val params = request.getJSONObject("params") | ||
| assertEquals(toolName, params.getString("name")) | ||
|
|
There was a problem hiding this comment.
The test builds a tool call request with a fixed ID of 123, but in the actual implementation (McpClientService.callToolInternal), the ID is generated using System.currentTimeMillis(). This inconsistency between test and production code could mask issues. Consider using the same ID generation strategy in tests or documenting why they differ.
| // Use fixed ID for deterministic test behavior | |
| val request = JSONObject().apply { | |
| put("jsonrpc", "2.0") | |
| put("id", 123L) | |
| put("method", "tools/call") | |
| put("params", JSONObject().apply { | |
| put("name", toolName) | |
| put("arguments", arguments) | |
| }) | |
| } | |
| assertEquals("2.0", request.getString("jsonrpc")) | |
| assertEquals(123L, request.getLong("id")) | |
| assertEquals("tools/call", request.getString("method")) | |
| val params = request.getJSONObject("params") | |
| assertEquals(toolName, params.getString("name")) | |
| // Use same ID generation strategy as production while keeping the test deterministic | |
| val requestId = System.currentTimeMillis() | |
| val request = JSONObject().apply { | |
| put("jsonrpc", "2.0") | |
| put("id", requestId) | |
| put("method", "tools/call") | |
| put("params", JSONObject().apply { | |
| put("name", toolName) | |
| put("arguments", arguments) | |
| }) | |
| } | |
| assertEquals("2.0", request.getString("jsonrpc")) | |
| assertEquals(requestId, request.getLong("id")) | |
| assertEquals("tools/call", request.getString("method")) | |
| val params = request.getJSONObject("params") | |
| assertEquals(toolName, params.getString("name")) |
| fun sanitizeIdentifier(value: String): String { | ||
| return value.lowercase() | ||
| .replace(Regex("[^a-z0-9]+"), "_") | ||
| .trim('_') | ||
| } |
There was a problem hiding this comment.
The sanitizeIdentifier method converts to lowercase and replaces non-alphanumeric characters with underscores, then trims underscores from the ends. However, if the input contains consecutive non-alphanumeric characters, this will create consecutive underscores in the middle of the identifier. For example, "My--Tool" becomes "my__tool". Consider replacing multiple consecutive underscores with a single underscore for cleaner identifiers.
|
@Godzilla675 if possible can you share a working apk ? No need to sign it just a debug one |
|
@Godzilla675 |
|
Till then add a MCP store or something + try including Termux like terminal, so users can run python in phone so more mcp servers get enabled |
|
@Siddhesh2377 hi I'm happy to help. Do you still need the debug apk? |
|
Not now I built it, just add those features, and I would love to merge it, also update your branch with my changes |
|
and for the MCP store should I add remote MCP servers like Zapier? or do you mean local servers? |
|
Can you join a Google meet ?, if you are comfortable ? |
|
@Siddhesh2377 hi
I am sorry but I would like not to. |
- Fix sanitizeIdentifier creating double underscores for consecutive special characters (e.g., 'My--Tool' now correctly becomes 'my_tool') - Add text-parsing fallback when native grammar fails to emit ToolCall events, preventing raw JSON from being displayed to users - Fix race condition in executeToolCall by retrying syncMcpTools once if tool registry lookup returns null - Clear stale tools in syncMcpTools catch block to prevent broken state - Fix singleton close() not shutting down OkHttpClient dispatcher (which made subsequent requests fail permanently) - Validate SSE parsed data has JSON-RPC fields before accepting - Improve listTools error logging with server name context - Add tests for sanitizeIdentifier edge cases Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- MCP Store screen with browsable registry of MCP servers - Fetches registry from remote GitHub URL, falls back to bundled JSON - Category filtering, search, one-tap install to Room database - Badges for API key requirements and Termux dependencies - Termux integration for running local Python MCP servers - TermuxBridge utility: detect Termux, run commands via RUN_COMMAND intent - pip install flow for Python-based MCP servers - Auto-configure localhost URLs for local servers - Setup dialog guides users to install Termux if not present - Database migration v5→v6: add isLocal and sourceStoreId columns - Navigation: McpStore route + Store button on McpServersScreen top bar - Registry seeded with 10 popular MCP servers (Brave, GitHub, DuckDuckGo, etc.) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… system Merge upstream changes (Character Cards, AI Memory, Plugin system, TTS, multi-turn generation) while preserving MCP server support, MCP Store, and Termux integration. Key conflict resolutions: - AppDatabase: bump to v7, add MIGRATION_6_7 for MCP tables alongside upstream's persona/ai_memory migrations (4→5, 5→6) - ChatViewModel: take upstream's PluginManager agent loop, add MCP tool registry and execution fallback in agent loop - Converters: keep both McpTransportType and StringList converters - MainActivity: merge MCP and upstream screen routes (Personas, AiMemory, Settings, McpServers, McpStore) - HomeDrawerScreen/HomeScreen: add MCP servers button alongside upstream's chatViewModel and onCharacterClick params - LlmModelWorker: take upstream's new tool calling API (enableToolCallingGguf, multi-turn generation) - ChatViewModelFactory: take upstream's Context param (MCP deps accessed via AppContainer) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This pull request introduces major new features and infrastructure for tool/plugin management (MCP servers) in the ToolNeuron Android app, along with improvements to build/test setup and permissions. The most important changes are the addition of MCP server support (including database schema, UI navigation, and a registry of available servers), a new GitHub Actions workflow for building debug APKs, and updates to permissions for Termux integration. Some code comments and onboarding logic have also been streamlined for clarity.
MCP Server (Tool Plugin) Support
McpServertable to the Room database schema (AppDatabase) and corresponding DAO for managing installed MCP servers; database version bumped to 7. [1] [2]McpServersScreen,McpStoreScreen) and integrated them into the app's navigation flow. [1] [2] [3] [4]mcp-registry.jsonasset listing built-in and community MCP servers/tools, including metadata and setup instructions.Build & Test Infrastructure
build-debug-apk.yml) to automatically build and upload debug APKs for pull requests and manual dispatch.build.gradle.ktsto include JUnit and org.json as test dependencies.Permissions & Platform Integration
AndroidManifest.xmlto addcom.termux.permission.RUN_COMMANDand package visibility for Termux, enabling integration with Termux-based MCP servers.Codebase Cleanliness & Comments
Documentation
.github/copilot-instructions.mdwith build, test, and architecture notes for contributors and Copilot.